Feat/V5 WIP #74
Feat/V5 WIP #74
Conversation
AdriGeorge
requested review from
alexcos20,
bogdanfazakas,
jamiehewitt15,
mariacarmina and
paulo-ocean
as code owners
src/commands.ts
Outdated
| console.error(e); | ||
| return; | ||
| let id: string | ||
| switch (asset.version) { |
There was a problem hiding this comment.
wouldn´t be easier to check if is v5 and then do something different, otherwise just do as before? Not really sure why a need a switch/case for only 2 options?
There was a problem hiding this comment.
because can be multiple versions in the future, but if is better with if/else, i will update it
There was a problem hiding this comment.
the thing is that, IMO, you're doing a lot of refactoring for introducing a small variation.. This goes for the CLI and the Nodes.. So it's hard to even review it, because there are many file changes
| const assetOwner = asset.nft.owner | ||
| let serviceType | ||
| if (isVerifiableCredential(asset)) { | ||
| serviceType = (asset as any).credentialSubject.services[0].type |
There was a problem hiding this comment.
IMO, we're doing this on the wrong way
we're pretty much force setting all the types to be "any" because it relies on ocean.jsupdates that were not done yet, like the Property 'credentialSubject' on 'Asset' for instance. These are workarounds. It should be the other way around... If is just a matter of adding a property we could easily update the sdk first.
FYI, we're introducing other ocean.js changes targeting next version branch (will be 4.0 with eventually some breaking changes) not main branch..
In any case, i think we should have an updated version of ocean.js before introducing these changes on the CLI and the node
On another note, take a look at this ongoing PR: #73, the idea is that we start using the createAsset function from the SDK, so eventually many of the changes we're doing here would eventually at some point move to the sdk (and be transparent)
There was a problem hiding this comment.
ok, thanks, i agree
|
We should have some documentation somewhere on what is the V5 DDO and what is it for? People might get confused that this is some sort of V5 release of Ocean which will cause confusion. It would actually be better if we found a different name for these changes - as we will most likely want to reserve the V5 title for if and when we do a V5 release. |
We have a pr in review with documentation: https://github.com/OceanProtocolEnterprise/docs-internal/pull/2 |
Your documentation is on a private repo. That's not going to help anyone who gets confused by these changes and thinks Occean V5 is on the way due to how you've named this. The simplest thing for you to do here is to change the name (ideally a name that indicates it is an enterprise version) and then add a small note in the readme saying "refer to the enterprise documentation for an explanation on how to work with the enterprise DDO version". |
Fixes # .
Changes proposed in this PR:
Need to fill this envs to use ipfs
export IPFS_API_KEY=
export IPFS_SECRET_API_KEY=
And this envs if sign with waltId:
export WALT_ID_ISSUER_API=
export ISSUER_iD=
export ISSUER_KTY=
export ISSUER_KEY_D=
export ISSUER_KEY_CRV=
export ISSUER_KEY_KID=
export ISSUER_KEY_X=
export SSI=true
work with OceanProtocolEnterprise/ocean-node#31